Hi @tmshn! Thanks for working on this.

GetOk is implemented the way it is because we can't distinguish between "unset" and "zero value" in some cases.

As you may have seen while working on this, internally ResourceData is an abstraction over states and diffs which tries to make all appear as a single data structure using a layered approach. If I am remembering correctly, this weird behavior of GetOk was done because the diff-based backend of ResourceData is unable to distinguish "unset" from "zero value", and so will never set r.Exists to false.

However, this change (to make GetOk return false for zero-values) was made a long time ago so honestly the reason for it has been somewhat forgotten and things may have changed in the mean time. I notice that ResourceAttrDiff has a NewRemoved field which could potentially represent this situation well enough for the ResourceData diff reader to deal with it.

Would you mind adding a few extra test cases here to test some of these diff-related cases?

• State has an optional, non-computed attribute set, and the diff has Old and NewRemoved set, signalling that the attribute is being removed. I think GetOkAllowZero should return ok = false in this case.
• State has an optional, non-computed attribute set, and the diff has Old and New set, with New set to the type's zero value. I think GetOkAllowZero should return ok = true in this case.

If the above two test cases work then in principle this change should work as expected, though given the number of Terraform subsystems that have to behave correctly for this to work we should probably also try to have a more "full-stack" test that tries the above scenarios with a config and a state, letting Terraform's core create the diff itself, just to make sure that the diffing code is producing the diffs the way we are assuming.

If you don't have time to look at the above yourself then please let me know! I'm happy to help out with getting this finished up when I have some spare cycles.

Thanks again for your time on this.

Hi @apparentlymart, thanks for the detailed explanation! But to be honest, I still don't understand why this ↓ happens...

the diff-based backend of ResourceData is unable to distinguish "unset" from "zero value", and so will never set r.Exists to false.

Anyway, I added the test cases you mentioned, and unfortunately the second one failed as you can see 😭
So... should I modify the method using d.getChange or something like that?

Hi again, @tmshn! Sorry for the delay in getting back to you here.

There has long been this rumor in the Terraform team that the behavior of GetOk returning false for zero values was due to some weird problem with diffs, but we'd collectively lost the detailed rationale for this behavior.

Given that your change here felt so reasonable, I decided that I needed to get to the bottom of why we made this change in the first place in order to see if we were missing something, and indeed it seems that we are. The full details are in #950.

The summary of the problem is that Terraform is inconsistent about how unspecified values are represented depending on whether they have never been set or whether they were formerly set and later removed from the configuration.

The behavior of GetOk was updated to return false for zero values to hide this inconsistency. This GetOkAllowZero method would not, it appears, get the correct result in the latter case.

I think that there is in principle a way to make this work as expected if we were to adjust some of the internals of how diffs are generated and how ResourceData deals with unsetting values in diffs, but in its current form I think GetOkAllowZero would behave in hard-to-predict ways depending on how a resource has changed over time. I'm worried about adding this in and having it appear to work in the basic case (creating a resource for the first time) but it then resulting in confusing behavior for scenarios where the user changes a resource that already exists.

So with all of that said, I'd prefer to hold on this PR for now. We have plans to rework how Terraform internally represents values across all of its subsystems, which will force us to revisit some of the core elements that contribute to this problem, at which point we can hopefully make some adjustments to support this use-case.

I very much appreciate the work you put into this, and indeed it was a good prompt for me to look into the core issue that led to this problem in the first place. I'm sorry that we can't merge this right now, but hopefully we can merge it in future once we've addressed some of the deeper challenges that led to this not being possible.

Thanks @apparentlymart, and sorry for my late reaction.

I totally agree that the problem should be fixed in the underlaying core part of the Terraform, to be able to handle diff correctly. I'll patiently wait for that 👍

So should I close this PR, or keep open?

Hi @tmshn! Thanks for your understanding and patience here.

I'm going to close this for now, since I think once we make the core changes I was referring to this set of changes would need some adjustments anyway, since I expect we will make some significant changes to the internals of helper/schema as part of that work.

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.